IoT-Safe: serialize APDU transactions with a port mutex - #11246
IoT-Safe: serialize APDU transactions with a port mutex#11246danielinux wants to merge 3 commits into
Conversation
All IoT-Safe operations share the file-scope static command/response buffers on a single CSIM channel. Concurrent operations (e.g. a random generation and an ECDSA sign callback from different TLS sessions) could overwrite each other's buffers and consume the wrong modem reply, corrupting cryptographic output or leaving the applet in a persistent error state. Add a port-level mutex (active in multi-threaded builds, no-op when SINGLE_THREADED) held across each APDU transaction. The seven single-transaction operations get thin serialized wrappers. GetRandom locks after its lazy-init check, and the ECDH callback locks its APDU branch, calling the locked impls directly since it already holds the mutex (the software fallback never takes the lock). The mutex is created in iotsafe_init(); document the single-threaded init contract in the doxygen of wolfSSL_CTX_iotsafe_enable() and the CSIM callback setters. Fixes F-10045.
There was a problem hiding this comment.
Pull request overview
This PR addresses concurrency hazards in the IoT-Safe port by serializing APDU transactions that share file-scope command/response buffers on a single CSIM channel, preventing concurrent TLS sessions from corrupting modem/appet state and cryptographic outputs.
Changes:
- Introduces a port-level mutex and lock helpers to serialize APDU transactions in multi-threaded builds (no-op under
SINGLE_THREADED). - Wraps single-transaction IoT-Safe operations with thin “serialized entry point” functions and updates the ECDH callback to hold the lock across its APDU sequence.
- Updates Doxygen to document the single-threaded initialization contract and concurrent-use guarantees after initialization.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| wolfcrypt/src/port/iotsafe/iotsafe.c | Adds mutex-based serialization for IoT-Safe APDU transactions and refactors operations into locked + serialized wrappers. |
| doc/dox_comments/header_files/iotsafe.h | Documents initialization/threading requirements for IoT-Safe enablement and CSIM callback setters. |
Suppressed comments (1)
wolfcrypt/src/port/iotsafe/iotsafe.c:101
- iotsafe_unlock() should mirror iotsafe_lock()'s initialization guard; otherwise callers that hit an early-return/no-op lock path can still attempt to unlock an uninitialized mutex in multi-threaded builds.
static void iotsafe_unlock(void)
{
#if !defined(SINGLE_THREADED)
wc_UnLockMutex(&iotsafe_mutex);
#endif
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- iotsafe_init(): free the mutex on the ATE0 and drain failure paths so a retried init never double-initializes it - guard every exported API entry (wc_iotsafe_*, wolfIoTSafe_GetCert_ex, wolfIoTSafe_GetRandom) against locking an uninitialized mutex via iotsafe_ensure_init(), which lazy-initializes the port before locking - doxygen: wolfIoTSafe_SetCSIM_write_cb param rf -> wf
wolfIoT_ecc_shared_secret(): ensure the port is initialized before the APDU branch can lock iotsafe_mutex. The check sits before tmpKey allocation (no leak on the early WC_HW_E return) and is gated on iotsafe->enabled so the software fallback path never forces init.
|
Can one of the admins verify this patch? |
|
jenkins retest this please |
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11246
Scan targets checked: wolfcrypt-port-bugs, wolfcrypt-rs-bugs
Findings: 4
2 finding(s) posted as inline comments (see file-level comments below)
Required changes (2)
GEN_KEYPAIR and READ_KEY are not serialized as one operation in wolfIoT_ecc_keygen
File: wolfcrypt/src/port/iotsafe/iotsafe.c:1217
Function: wolfIoT_ecc_keygen
Category: Race conditions
iotsafe_gen_keypair() and iotsafe_get_public_key() each acquire and release iotsafe_mutex separately. A concurrent session regenerating the same ecdh_keypair_slot between the two calls makes the exported public key inconsistent with the applet's stored private key, breaking the handshake. Adjacent to known finding #45, which concerns the shared static buffers rather than applet slot state.
Related known finding #10045 (similar but distinct): Both are concurrent IoT-Safe operation races, but #10045 concerns interleaving access to process-global APDU command/response buffers. This candidate concerns interleaving two individually serialized APDU operations that mutate and read the same applet key slot. It requires an operation-level lock spanning key generation and public-key retrieval, not merely the buffer-serialization patch.
Recommendation: Hold iotsafe_lock() across both steps and call the _locked variants, as wolfIoT_ecc_shared_secret() already does.
Referenced code: wolfcrypt/src/port/iotsafe/iotsafe.c:1217-1221 (5 lines)
PutPublic and VerifyHash are not serialized as one operation in wolfIoT_ecc_verify
File: wolfcrypt/src/port/iotsafe/iotsafe.c:1392
Function: wolfIoT_ecc_verify
Category: Race conditions
iotsafe_put_public_key() and iotsafe_verify_hash() take iotsafe_mutex independently, so another thread storing into the same peer key slot in between leaves the verification running against a different public key. Adjacent to known finding #45, which concerns the shared static buffers rather than applet slot state.
Related known finding #10045 (similar but distinct): Both are races in the IoT-Safe backend, but #10045 is shared host-side APDU buffer corruption between operations. This candidate is an applet-slot time-of-check/use race between separately locked PutPublic and VerifyHash calls. The affected operation sequence, state, and required transaction-level locking patch are distinct.
Recommendation: Hold iotsafe_lock() across the store-then-verify sequence and call the _locked variants.
Referenced code: wolfcrypt/src/port/iotsafe/iotsafe.c:1392-1395 (4 lines)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
| * Returns 0 on success, < 0 if initialization failed. */ | ||
| static int iotsafe_ensure_init(void) | ||
| { | ||
| if (!wolfIoT_initialized) |
There was a problem hiding this comment.
Lazy mutex creation in iotsafe_ensure_init() is itself racy · Race conditions
The check of wolfIoT_initialized and the following iotsafe_init() (which runs wc_InitMutex(&iotsafe_mutex)) are unsynchronized. Two threads whose first IoT-Safe call races both initialize the same static mutex, overwriting the handle and silently defeating the serialization this PR adds.
Related known finding #10045 (similar but distinct): Both affect iotsafe.c concurrency and involve unsynchronized initialization-related state, but #10045 races shared APDU buffers during operations, whereas this candidate races creation of the newly added mutex itself. The faulting operations, root causes, and fixes differ: transaction serialization does not make mutex initialization safe.
Fix: Create iotsafe_mutex once during library initialization (or use a statically initialized mutex) rather than lazily from every API entry.
| * Returns 0 on success, < 0 if initialization failed. */ | ||
| static int iotsafe_ensure_init(void) | ||
| { | ||
| if (!wolfIoT_initialized) |
There was a problem hiding this comment.
Lazy init added at every entry point is itself unserialized and can re-initialize a held mutex · Logic errors
iotsafe_ensure_init() runs iotsafe_init() unserialized, and wolfIoT_initialized is only set after the applet-load APDUs finish, so a concurrent first call re-runs wc_InitMutex() on a mutex another thread already holds and drives APDUs over the shared csim_cmd buffer.
Related known finding #10045 (similar but distinct): Both involve concurrent IoT-Safe use and the unsynchronized initialization state, but this finding faults in iotsafe_ensure_init re-running mutex initialization during first-use races; #10045 faults in concurrent APDU transactions overwriting shared command/response buffers. The operations and required initialization-versus-transaction serialization patches differ.
Fix: Use a statically initialized mutex (WOLFSSL_MUTEX_INITIALIZER) to guard init, or drop the lazy init and return WC_HW_E when uninitialized.
Description
All IoT-Safe operations share the file-scope static command/response buffers on a single CSIM channel. Concurrent operations (e.g. a random generation and an ECDSA sign callback from different TLS sessions) could overwrite each other's buffers and consume the wrong modem reply, corrupting cryptographic output or leaving the applet in a persistent error state.
Add a port-level mutex (active in multi-threaded builds, no-op when SINGLE_THREADED) held across each APDU transaction. The seven single-transaction operations get thin serialized wrappers. GetRandom locks after its lazy-init check, and the ECDH callback locks its APDU branch, calling the locked impls directly since it already holds the mutex (the software fallback never takes the lock). The mutex is created in iotsafe_init(); document the single-threaded init contract in the doxygen of wolfSSL_CTX_iotsafe_enable() and the CSIM callback setters.
Fixes F-10045.
Testing
Tested via existing iotsafe demos build
Checklist